Federation: Notify Remote Users of Being Added to a New Conversation#1594
Federation: Notify Remote Users of Being Added to a New Conversation#1594mdimjasevic merged 19 commits intodevelopfrom
Conversation
mdimjasevic
changed the title
mdimjasevic
force-pushed
the
mdimjasevic/fed-inform-remote-be-conv
branch
from
e35ef19 to
4bf3c28
Compare
pcapriotti
left a comment
pcapriotti
left a comment
There was a problem hiding this comment.
I'm not sure this is behaving correctly in terms of events. See comment below.
There was a problem hiding this comment.
This utility function is a bit confusing. Could you document what its arguments are? For example, it seems that alice is only used for its domain.
- Update both the local and remote databases with information on a new conversation - Notify both local and remote users of the new conversation
mdimjasevic
force-pushed
the
mdimjasevic/fed-inform-remote-be-conv
branch
from
b77da97 to
f1eea64
Compare
pcapriotti
left a comment
pcapriotti
left a comment
There was a problem hiding this comment.
This looks ok, but maybe it would have been slightly nicer to add a Maybe CreateConversation field to the updateConversationMemberships RPC, since they basically do the same thing, and the only difference is the events they send out.
I'm also not sure about the name CreateConversation, which sounds like an action, when it should be a noun. Maybe change it to CreatedConversation?
By the way, what is wrong with simply reusing the Conversation type from wire-api instead of introducing a new one?
- Get rid of a redundant record field - Rename a data type to better reflect its purpose -- Rename fields and functions accordingly
…evic/fed-inform-remote-be-conv
…evic/fed-inform-remote-be-conv
4 participants
This PR introduces:
Note 1: This depends on PR #1585.
Note 2: I am not confident in the test's correctness. I based it on @jschaul's similar test.